Skip to content

fix: Clean up sessions from manager when terminated via DELETE request #791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

1ytic
Copy link

@1ytic 1ytic commented May 23, 2025

Summary

This PR fixes a memory leak issue in StreamableHTTPSessionManager where sessions were not being cleaned up from the manager's internal dictionary when terminated via DELETE requests.

Problem

Previously, when clients sent DELETE requests to terminate sessions:

  • The StreamableHTTPServerTransport would mark itself as terminated
  • However, the StreamableHTTPSessionManager would continue to hold references to these terminated sessions in its _server_instances dictionary
  • This caused memory accumulation over time in long-running servers

Solution

Added a callback mechanism to properly clean up terminated sessions:

  • Added callback parameter to StreamableHTTPServerTransport constructor (on_session_terminated)
  • Implemented cleanup method in StreamableHTTPSessionManager (_on_session_terminated)
  • Enhanced termination flow to notify manager when sessions are terminated via DELETE
  • Fixed HTTP status codes to return 404 NOT_FOUND for unknown session IDs (was 400 BAD_REQUEST)

Technical Details

  1. When a DELETE request terminates a session, StreamableHTTPServerTransport._terminate_session() now calls the callback
  2. The callback removes the session from StreamableHTTPSessionManager._server_instances
  3. Subsequent requests with that session ID properly return 404 NOT_FOUND
  4. Memory is freed for garbage collection

Test Plan

  • Added comprehensive test test_session_cleanup_on_delete_request
  • Verified existing session termination tests still pass
  • Confirmed no regressions in broader test suite
  • Validated proper HTTP status codes (404 for unknown sessions)

Backward Compatibility

✅ This change is fully backward compatible - no breaking changes to existing APIs.

Generated with Claude Code

Previously, StreamableHTTPSessionManager would not remove sessions from its
_server_instances dictionary when clients sent DELETE requests to terminate
sessions. This caused terminated sessions to remain in memory until server
shutdown, leading to potential memory accumulation in long-running servers.

This change adds a callback mechanism between StreamableHTTPServerTransport
and StreamableHTTPSessionManager to properly clean up terminated sessions:

- Add on_session_terminated callback parameter to StreamableHTTPServerTransport
- Implement _on_session_terminated method in StreamableHTTPSessionManager
- Call callback from _terminate_session when DELETE request terminates session
- Update HTTP status codes: return 404 NOT_FOUND for unknown session IDs
- Add comprehensive test to verify session cleanup functionality

Sessions are now automatically removed from the manager's memory when explicitly
terminated via DELETE requests, preventing memory leaks while maintaining
backward compatibility.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ihrpr ihrpr added this to the r-06-25 milestone May 23, 2025
@1ytic 1ytic closed this May 26, 2025
@1ytic 1ytic deleted the fix/session-cleanup-on-delete branch May 26, 2025 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants